fix: resolve Windows Bun virtual FS paths for Claude CLI subprocess#1091
fix: resolve Windows Bun virtual FS paths for Claude CLI subprocess#1091
Conversation
…1087) On Windows, the compiled Archon binary embeds cli.js at B:/~BUN/root/ instead of $bunfs/. The SDK's extractFromBunfs only detects $bunfs paths, so Windows paths pass through unchanged — child processes can't access the parent's virtual FS, causing "Module not found" exit code 1. Changes: - Add resolveWindowsBunfsCliPath() to extract ~BUN paths to real temp files - Use resolvedCliPath instead of raw cliPath for pathToClaudeCodeExecutable - Add tests for path detection, extraction, and fallback behavior Fixes #1087 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔍 Comprehensive PR ReviewPR: #1091 — fix: resolve Windows Bun virtual FS paths for Claude CLI subprocess SummaryThis is a well-implemented, focused Windows compatibility fix that mirrors the upstream SDK logic faithfully. The implementation is correct, all 4 logical branches are tested, and scope discipline is excellent (zero unrelated changes). No critical or high-severity issues were found. Two quick MEDIUM fixes are recommended before marking ready. Verdict:
🟡 Medium Issues (Recommend Fixing — Quick Wins)1.
|
| Issue | Location | Agent | Suggestion |
|---|---|---|---|
| Node built-in imports placed after project imports | claude.ts:46-49 |
code-review | Move fs, path, os, crypto to top of import block (before SDK/@archon imports) |
Test uses require() (CJS) instead of ESM import |
claude.test.ts:1124-1126 |
code-review | Add top-level ESM import for fs, os, path at file top |
(err as Error).message loses stack trace |
claude.ts:84 |
error-handling | Pass full err object; if keeping console.warn, use String(err) defensively |
No assertion that console.warn fires in fallback test |
claude.test.ts |
test-coverage | Add spyOn(console, 'warn') assertion for regression-proofing the diagnostic |
Inline guard comment slightly overstates extractFromBunfs guarantee |
claude.ts:62-64 |
comment-quality | Change "are already resolved" → "are expected to have been resolved already" |
✅ What's Good
- Mirrors upstream SDK faithfully — implementation is nearly identical to
extractFromBunfs.js, minimizing divergence risk and making intent immediately clear to reviewers - Atomic write pattern —
writeFileSync→chmodSync→renameSyncprevents partial-read races, same as upstream - Content-hash temp dir — reuses the same extracted file for the same binary version, no temp file accumulation across restarts
- All 4 branches tested — pass-through (non-Windows),
$bunfspass-through, successful extraction with content integrity verified, and error fallback - Real FS tests are justified — Bun's module binding means
spyOn(fs, 'readFileSync')can't intercept named imports; real FS tests are more reliable here (documented in scope) - Fallback is intentional and documented — catch block comment explains why throwing is impossible at module init time, matching CLAUDE.md's requirement to document intentional fail-open
- Correct test batching — no new
mock.module()calls; slots into existingclaude.test.tsbatch withoutpackage.jsonchanges
📋 Suggested Follow-up Issues
| Issue Title | Priority |
|---|---|
Add console.warn assertion to Windows bunfs fallback test |
P3 |
Normalize import ordering in claude.ts |
P3 |
Next Steps
- ⚡ Fix the 2 MEDIUM issues (ideally by switching
console.warn→getLog().warn(...)which resolves both) - 📝 LOW issues are optional — consider deferring to follow-up
- 🎯 Mark ready and merge when CI is green
Reviewed by Archon comprehensive-pr-review workflow
Artifacts: ~/.archon/workspaces/coleam00/Archon/artifacts/runs/cca84316788e97e0481c92609c58fad2/review/
- Move Node built-in imports (fs, path, os, crypto) before SDK/project imports - Switch console.warn to getLog().warn() with structured context and correct event name (claude.windows_bunfs_extraction_failed), fixing the wrong $bunfs label in the warning message and aligning with project Pino logging conventions - Soften inline guard comment: "are already resolved" -> "are expected to have been resolved already" to avoid overstating the extractFromBunfs guarantee - Add module-level comment for resolvedCliPath explaining the once-at-init pattern - Replace require() CJS calls in test with top-level ESM imports (fs, os, path) - Add assertion in fallback test that mockLogger.warn fires with the correct event name, regression-proofing the diagnostic Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fix Report: PR #1091Date: 2026-04-11T00:00:00Z SummaryAll 7 review findings were addressed (2 MEDIUM + 5 LOW). The primary fix switches the catch block from Fixes Applied
Validation
|
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Archon PR Validation ReportVerdict: REQUEST_CHANGES SummaryThe fix correctly solves the Windows Bun virtual FS path issue with a clean, minimal approach mirroring the SDK's own extraction pattern. One blocking issue: a Temporal Dead Zone (TDZ) ordering bug would crash the module on Windows when extraction fails. Simple fix — reorder declarations. Bug Confirmation
IssuesMust fix (HIGH): Nice to have (LOW): Add What's Done Well
Validated by archon-validate-pr workflow |
…indows When extraction fails, getLog() was called before its declaration, causing a ReferenceError. Found by validate-pr workflow. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…esolver Drop `@anthropic-ai/claude-agent-sdk/embed` and resolve Claude Code via CLAUDE_BIN_PATH env → assistants.claude.claudeBinaryPath config → throw with install instructions. The embed's silent failure modes on macOS (#1210) and Windows (#1087) become actionable errors with a documented recovery path. Dev mode (bun run) remains auto-resolved via node_modules. The setup wizard auto-detects Claude Code by probing the native installer path (~/.local/bin/claude), npm global cli.js, and PATH, then writes CLAUDE_BIN_PATH to ~/.archon/.env. Dockerfile pre-sets CLAUDE_BIN_PATH so extenders using the compiled binary keep working. Release workflow gets negative and positive resolver smoke tests. Docs, CHANGELOG, README, .env.example, CLAUDE.md, test-release and archon skills all updated to reflect the curl-first install story. Retires #1210, #1087, #1091 (never merged, now obsolete). Implements #1176.
|
Filing context: #1217 is landing as a different approach to the same class of bug this draft targets. Instead of extracting The net effect is that #1217 fixes #1087 (which this draft was targeting) and #1210 (the macOS manifestation that surfaced after this draft) without needing the Windows-only extractor. Leaving this PR open for you to close at your convenience — not trying to auto-close on merge. |
…solver (#1217) * feat(providers): replace Claude SDK embed with explicit binary-path resolver Drop `@anthropic-ai/claude-agent-sdk/embed` and resolve Claude Code via CLAUDE_BIN_PATH env → assistants.claude.claudeBinaryPath config → throw with install instructions. The embed's silent failure modes on macOS (#1210) and Windows (#1087) become actionable errors with a documented recovery path. Dev mode (bun run) remains auto-resolved via node_modules. The setup wizard auto-detects Claude Code by probing the native installer path (~/.local/bin/claude), npm global cli.js, and PATH, then writes CLAUDE_BIN_PATH to ~/.archon/.env. Dockerfile pre-sets CLAUDE_BIN_PATH so extenders using the compiled binary keep working. Release workflow gets negative and positive resolver smoke tests. Docs, CHANGELOG, README, .env.example, CLAUDE.md, test-release and archon skills all updated to reflect the curl-first install story. Retires #1210, #1087, #1091 (never merged, now obsolete). Implements #1176. * fix(providers): only pass --no-env-file when spawning Claude via Bun/Node `--no-env-file` is a Bun flag that prevents Bun from auto-loading `.env` from the subprocess cwd. It is only meaningful when the Claude Code executable is a `cli.js` file — in which case the SDK spawns it via `bun`/`node` and the flag reaches the runtime. When `CLAUDE_BIN_PATH` points at a native compiled Claude binary (e.g. `~/.local/bin/claude` from the curl installer, which is Anthropic's recommended default), the SDK executes the binary directly. Passing `--no-env-file` then goes straight to the native binary, which rejects it with `error: unknown option '--no-env-file'` and the subprocess exits code 1. Emit `executableArgs` only when the target is a `.js` file (dev mode or explicit cli.js path). Caught by end-to-end smoke testing against the curl-installed native Claude binary. * docs: record env-leak validation result in provider comment Verified end-to-end with sentinel `.env` and `.env.local` files in a workflow CWD that the native Claude binary (curl installer) does not auto-load `.env` files. With Archon's full spawn pathway and parent env stripped, the subprocess saw both sentinels as UNSET. The first-layer protection in `@archon/paths` (#1067) handles the inheritance leak; `--no-env-file` only matters for the Bun-spawned cli.js path, where it is still emitted. * chore(providers): cleanup pass — exports, docs, troubleshooting Final-sweep cleanup tied to the binary-resolver PR: - Mirror Codex's package surface for the new Claude resolver: add `./claude/binary-resolver` subpath export and re-export `resolveClaudeBinaryPath` + `claudeFileExists` from the package index. Renames the previously single `fileExists` re-export to `codexFileExists` for symmetry; nothing outside the providers package was importing it. - Add a "Claude Code not found" entry to the troubleshooting reference doc with platform-specific install snippets and pointers to the AI Assistants binary-path section. - Reframe the example claudeBinaryPath in reference/configuration.md away from cli.js-only language; it accepts either the native binary or cli.js. * test+refactor(providers, cli): address PR review feedback Two test gaps and one doc nit from the PR review (#1217): - Extract the `--no-env-file` decision into a pure exported helper `shouldPassNoEnvFile(cliPath)` so the native-binary branch is unit testable without mocking `BUNDLED_IS_BINARY` or running the full sendQuery pathway. Six new tests cover undefined, cli.js, native binary (Linux + Windows), Homebrew symlink, and suffix-only matching. Also adds a `claude.subprocess_env_file_flag` debug log so the security-adjacent decision is auditable. - Extract the three install-location probes in setup.ts into exported wrappers (`probeFileExists`, `probeNpmRoot`, `probeWhichClaude`) and export `detectClaudeExecutablePath` itself, so the probe order can be spied on. Six new tests cover each tier winning, fall-through ordering, npm-tier skip when not installed, and the which-resolved-but-stale-path edge case. - CLAUDE.md `claudeBinaryPath` placeholder updated to reflect that the field accepts either the native binary or cli.js (the example value was previously `/absolute/path/to/cli.js`, slightly misleading now that the curl-installer native binary is the default). Skipped from the review by deliberate scope decision: - `resolveClaudeBinaryPath` async-with-no-await: matches Codex's resolver signature exactly. Changing only Claude breaks symmetry; if pursued, do both providers in a separate cleanup PR. - `isAbsolute()` validation in parseClaudeConfig: Codex doesn't do it either. Resolver throws on non-existence already. - Atomic `.env` writes in setup wizard: pre-existing pattern this PR touched only adjacently. File as separate issue if needed. - classifyError branch in dag-executor for setup errors: scope creep. - `.env.example` "missing #" claim: false positive (verified all CLAUDE_BIN_PATH lines have proper comment prefixes). * fix(test): use path.join in Windows-compatible probe-order test The "tier 2 wins (npm cli.js)" test hardcoded forward-slash path comparisons, but `path.join` produces backslashes on Windows. Caused the Windows CI leg of the test suite to fail while macOS and Linux passed. Use `path.join` for both the mock return value and the expectation so the separator matches whatever the platform produces.
|
Closing — the underlying bug was real (#1087), but PR #1217 (merged April 14) already resolved this with a better approach: it removed the Since there's no remaining code path using Thanks for the solid investigation and fix — the approach was correct, just superseded by #1217. |
…solver (coleam00#1217) * feat(providers): replace Claude SDK embed with explicit binary-path resolver Drop `@anthropic-ai/claude-agent-sdk/embed` and resolve Claude Code via CLAUDE_BIN_PATH env → assistants.claude.claudeBinaryPath config → throw with install instructions. The embed's silent failure modes on macOS (coleam00#1210) and Windows (coleam00#1087) become actionable errors with a documented recovery path. Dev mode (bun run) remains auto-resolved via node_modules. The setup wizard auto-detects Claude Code by probing the native installer path (~/.local/bin/claude), npm global cli.js, and PATH, then writes CLAUDE_BIN_PATH to ~/.archon/.env. Dockerfile pre-sets CLAUDE_BIN_PATH so extenders using the compiled binary keep working. Release workflow gets negative and positive resolver smoke tests. Docs, CHANGELOG, README, .env.example, CLAUDE.md, test-release and archon skills all updated to reflect the curl-first install story. Retires coleam00#1210, coleam00#1087, coleam00#1091 (never merged, now obsolete). Implements coleam00#1176. * fix(providers): only pass --no-env-file when spawning Claude via Bun/Node `--no-env-file` is a Bun flag that prevents Bun from auto-loading `.env` from the subprocess cwd. It is only meaningful when the Claude Code executable is a `cli.js` file — in which case the SDK spawns it via `bun`/`node` and the flag reaches the runtime. When `CLAUDE_BIN_PATH` points at a native compiled Claude binary (e.g. `~/.local/bin/claude` from the curl installer, which is Anthropic's recommended default), the SDK executes the binary directly. Passing `--no-env-file` then goes straight to the native binary, which rejects it with `error: unknown option '--no-env-file'` and the subprocess exits code 1. Emit `executableArgs` only when the target is a `.js` file (dev mode or explicit cli.js path). Caught by end-to-end smoke testing against the curl-installed native Claude binary. * docs: record env-leak validation result in provider comment Verified end-to-end with sentinel `.env` and `.env.local` files in a workflow CWD that the native Claude binary (curl installer) does not auto-load `.env` files. With Archon's full spawn pathway and parent env stripped, the subprocess saw both sentinels as UNSET. The first-layer protection in `@archon/paths` (coleam00#1067) handles the inheritance leak; `--no-env-file` only matters for the Bun-spawned cli.js path, where it is still emitted. * chore(providers): cleanup pass — exports, docs, troubleshooting Final-sweep cleanup tied to the binary-resolver PR: - Mirror Codex's package surface for the new Claude resolver: add `./claude/binary-resolver` subpath export and re-export `resolveClaudeBinaryPath` + `claudeFileExists` from the package index. Renames the previously single `fileExists` re-export to `codexFileExists` for symmetry; nothing outside the providers package was importing it. - Add a "Claude Code not found" entry to the troubleshooting reference doc with platform-specific install snippets and pointers to the AI Assistants binary-path section. - Reframe the example claudeBinaryPath in reference/configuration.md away from cli.js-only language; it accepts either the native binary or cli.js. * test+refactor(providers, cli): address PR review feedback Two test gaps and one doc nit from the PR review (coleam00#1217): - Extract the `--no-env-file` decision into a pure exported helper `shouldPassNoEnvFile(cliPath)` so the native-binary branch is unit testable without mocking `BUNDLED_IS_BINARY` or running the full sendQuery pathway. Six new tests cover undefined, cli.js, native binary (Linux + Windows), Homebrew symlink, and suffix-only matching. Also adds a `claude.subprocess_env_file_flag` debug log so the security-adjacent decision is auditable. - Extract the three install-location probes in setup.ts into exported wrappers (`probeFileExists`, `probeNpmRoot`, `probeWhichClaude`) and export `detectClaudeExecutablePath` itself, so the probe order can be spied on. Six new tests cover each tier winning, fall-through ordering, npm-tier skip when not installed, and the which-resolved-but-stale-path edge case. - CLAUDE.md `claudeBinaryPath` placeholder updated to reflect that the field accepts either the native binary or cli.js (the example value was previously `/absolute/path/to/cli.js`, slightly misleading now that the curl-installer native binary is the default). Skipped from the review by deliberate scope decision: - `resolveClaudeBinaryPath` async-with-no-await: matches Codex's resolver signature exactly. Changing only Claude breaks symmetry; if pursued, do both providers in a separate cleanup PR. - `isAbsolute()` validation in parseClaudeConfig: Codex doesn't do it either. Resolver throws on non-existence already. - Atomic `.env` writes in setup wizard: pre-existing pattern this PR touched only adjacently. File as separate issue if needed. - classifyError branch in dag-executor for setup errors: scope creep. - `.env.example` "missing #" claim: false positive (verified all CLAUDE_BIN_PATH lines have proper comment prefixes). * fix(test): use path.join in Windows-compatible probe-order test The "tier 2 wins (npm cli.js)" test hardcoded forward-slash path comparisons, but `path.join` produces backslashes on Windows. Caused the Windows CI leg of the test suite to fail while macOS and Linux passed. Use `path.join` for both the mock return value and the expectation so the separator matches whatever the platform produces.
2 participants
Summary
pathToClaudeCodeExecutableto a Bun virtual FS path (B:/~BUN/root/cli-xxx.js) that child processes cannot access, causing exit code 1 and "Module not found" errorsextractFromBunfsutility only detects$bunfs(Linux/Mac) paths and silently passes WindowsB:/~BUN/root/paths through unchangedresolveWindowsBunfsCliPath()topackages/core/src/clients/claude.tsthat detects Windows Bun FS paths by the~BUNmarker and extracts them to a real temp file using the same atomic write pattern asextractFromBunfsChanges
packages/core/src/clients/claude.ts: AddedresolveWindowsBunfsCliPath()function andresolvedCliPathconstant;pathToClaudeCodeExecutablenow usesresolvedCliPathinstead of rawcliPath; updated embed import comment to document the Windows workaroundpackages/core/src/clients/claude.test.ts: Added tests forresolveWindowsBunfsCliPathcovering pass-through for non-Windows paths, Windows path extraction, and fallback behavior on extraction failureValidation
All checks passed on first run:
bun run type-checkbun run lintbun run format:checkbun run testRoot Cause
The Archon binary cross-compiles on Linux for Windows (
--target bun-windows-x64). At compile time,import cliPath from '@anthropic-ai/claude-agent-sdk/embed'embedscli.jsinto the binary's virtual FS. On Linux/Mac binaries the embedded path format is$bunfs/...; on Windows binaries the format isB:/~BUN/root/.... The SDK'sextractFromBunfs.jshardcodes a$bunfscheck and returns Windows virtual FS paths unchanged — child processes then fail to find the module since they cannot access the parent's virtual FS.Fixes #1087